Skip to content

Harden bvar sampling against UAF - #3487

Merged
wwbmmm merged 1 commit into
apache:masterfrom
chenBright:harden_bvar_sampler
Aug 25, 2026
Merged

Harden bvar sampling against UAF#3487
wwbmmm merged 1 commit into
apache:masterfrom
chenBright:harden_bvar_sampler

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

bvar sampling has two lifetime hazards, both of which end up as a silent
use-after-free:

  1. detail::ReducerSampler holds a raw R* _reducer and dereferences it in
    take_sample() and get_value(). Safety therefore relies entirely on every
    host correctly calling Sampler::destroy() from its dtor, which is a convention,
    not a structural guarantee.

  2. Window/PerSecond borrow the sampler of the bvar they reference
    (var->get_sampler()), and WindowBase::SeriesSampler::Op holds a raw
    R* _var. If a Window outlives that bvar (violating the contract documented
    in bvar/window.h), the sampling thread has already deleted the sampler, so
    the Window is left with a permanently dangling pointer: both get_value() and
    the series sampler become a use-after-free, and nothing reports the misuse.

What is changed and the side effects?

Changed:

  1. Sample through the shared data carrier where possible.
    ReducerSampler now selects its data source with a trait: hosts exposing
    share_combiner() are sampled through their shared_ptr<AgentCombiner> plus
    by-value copies of Op/InvOp, so the sampler never dereferences the host after
    construction. Since the sampler keeps a reference to the combiner, sampling reads
    valid memory even if the host is destructed before the sampler is recycled.
    share_combiner() is added to Reducer (Adder/Maxer/Miner), IntRecorder and
    Percentile (the one inside LatencyRecorder). Hosts without such a carrier --
    PassiveStatus (data lives in a user callback) and the babylon variants (value
    types) -- keep the previous host-pointer mode, so their behaviour is unchanged.

  2. Detect a Window outliving its bvar, and degrade UAF to a bounded leak.
    Sampler gains a borrower counter (guarded by its existing _mutex) with
    add_borrower()/remove_borrower(), called by WindowBase's ctor/dtor. If
    destroy() finds the sampler still borrowed, it reports the misuse (including
    the bvar name) and marks the sampler, and the sampling thread then skips the
    delete, leaking it on purpose. The borrowers keep pointing at valid memory
    and merely stop receiving new samples. Note the counter and the "don't delete"
    part are inseparable: otherwise remove_borrower() itself would be a
    use-after-free. A new gflag bvar_abort_on_sampler_still_borrowed (default
    false, i.e. LOG(ERROR)) can escalate this to an abort, mirroring the existing
    bvar_abort_on_same_name.

3. Stop touching the var from the series sampler.
WindowBase::SeriesSampler::Op used to hold R* _var and call _var->op() from
the sampling thread. It now holds a copy of the operator (bvar operators such as
AddTo/MaxTo/AddStat are stateless functors), copied once into WindowBase::_var_op
at construction. As a result _var is only dereferenced in the ctor and never afterwards.

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

@chenBright
chenBright requested a lite review from Copilot August 24, 2026 16:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Hardens bvar sampling against silent use-after-free by making sampling independent of host lifetime where possible and by detecting/leaking borrowed samplers when Window/PerSecond outlive their referenced bvar.

Changes:

  • Teach ReducerSampler to sample via a shared combiner (when available) instead of dereferencing the host after construction.
  • Add borrower tracking + diagnostics to Sampler to detect Window/PerSecond outliving a referenced bvar and intentionally leak the sampler to avoid UAF.
  • Update WindowBase series sampling to copy the operator instead of touching the underlying var from the sampling thread; add a regression test for the lifetime violation case.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/bvar_window_unittest.cpp Adds regression test for Window/PerSecond outliving referenced bvar (skipped under ASan due to intentional leak).
test/bvar_sampler_unittest.cpp Removes unused includes related to sampler tests.
src/bvar/window.h Copies underlying var operator, adds sampler borrower tracking calls, and adjusts series sampler to avoid dereferencing destroyed vars.
src/bvar/reducer.h Adds share_combiner() + sampler debug name wiring for better lifetime safety and diagnostics.
src/bvar/recorder.h Adds share_combiner(), improves sampler debug naming, and updates expose/debug-name propagation.
src/bvar/passive_status.h Sets sampler debug name and updates expose path to refresh it when available.
src/bvar/latency_recorder.cpp Adds compile-time checks validating share-combiner detection behavior.
src/bvar/detail/sampler.h Adds borrower tracking + debug name, and introduces share-combiner-based sampling sources for ReducerSampler.
src/bvar/detail/sampler.cpp Implements borrower tracking/leak behavior, adds a gflag, and respects _leaked in collector deletion.
src/bvar/detail/percentile.h Adds share_combiner() and propagates debug names to sampler instances.
src/brpc/input_messenger.cpp Fixes indentation for an existing error message call.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/bvar/detail/sampler.cpp Outdated
Comment thread src/bvar/detail/sampler.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Comment thread src/bvar/detail/sampler.cpp
@chenBright
chenBright force-pushed the harden_bvar_sampler branch from 90902e3 to 6b8738c Compare August 25, 2026 03:21
@chenBright chenBright changed the title Harden bvar sampling against use-after-free Harden bvar sampling against UAF Aug 25, 2026
@wwbmmm
wwbmmm merged commit 98f42bc into apache:master Aug 25, 2026
15 checks passed
@chenBright
chenBright deleted the harden_bvar_sampler branch August 25, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants